feat: automate ERC20 automation #6665
Conversation
modules/bitgo/src/v2/coinFactory.ts
Outdated
| .filter((coin) => coin.features.includes(CoinFeature.SUPPORTS_ERC20)) | ||
| .forEach((coin) => { | ||
| const coinNames = { | ||
| [coin.network.type]: coin.name, |
There was a problem hiding this comment.
would only populate for Mainnet
modules/bitgo/src/v2/coinFactory.ts
Outdated
| Mainnet: `${coin.network.name}`, | ||
| Testnet: `t${coin.network.name}`, |
There was a problem hiding this comment.
Should this be network name or coin name?
There was a problem hiding this comment.
As discussed, would fetch this from coin.name now.
modules/sdk-coin-evm/src/register.ts
Outdated
| .forEach((coin) => { | ||
| //TODO: Decide if it should stay here, maybe move to network or coin itself. | ||
| const coinNames = { | ||
| Mainnet: `${coin.network.name}`, | ||
| Testnet: `t${coin.network.name}`, | ||
| }; | ||
|
|
||
| EthLikeErc20Token.createTokenConstructors(coinNames).forEach(({ name, coinConstructor }) => { | ||
| sdk.register(name, coinConstructor); |
There was a problem hiding this comment.
This'll end up calling createTokenConstructors multiple time. One for each token.
I don't think that's the expected behavior
There was a problem hiding this comment.
A valid catch. 👍
Since it was implemented on tokens instead of parent coin, the loop would call all tokens and register all tokens for each individual tokens.
Since we've moved the approach to parent coin instead of tokens, this would be fine now.
modules/sdk-coin-evm/src/register.ts
Outdated
| @@ -20,6 +21,20 @@ export const register = (coinFamily: string, sdk: BitGoBase): void => { | |||
| .forEach((coin) => { | |||
| sdk.register(coin.name, EvmCoin.createInstance); | |||
| }); | |||
| //TODO: add token registration after EVM Token Optimisation is implemented | |||
| } | |||
| if (coins.get(coinFamily).features.includes(CoinFeature.IS_ERC20_TOKEN)) { | |||
There was a problem hiding this comment.
If we want to add the feature to coin, then let's rework the feature name.
Current feature name suggests the coin is token as well.
There was a problem hiding this comment.
Idea was to use this flag with tokens instead of parent coin, hence the name IS_ERC20_TOKEN. I've now updated it to SUPPORTS_ERC20.
modules/statics/src/account.ts
Outdated
| export class EthLikeERC20Token extends ContractAddressDefinedToken { | ||
| public readonly coinNames: { Mainnet: string; Testnet: string }; | ||
|
|
||
| constructor(options: Erc20ConstructorOptions & { coinNames: { Mainnet: string; Testnet: string } }) { |
There was a problem hiding this comment.
Why take this as constructor params. Evaluate if we can derive this from the Network
There was a problem hiding this comment.
Removed from constructors and static properties. It'd be fetched from coin name now. As discussed, we would not make this part of the network property as it's the same as coin.name in all of our use cases.
modules/sdk-coin-evm/src/register.ts
Outdated
| if (coinFeatures.includes(CoinFeature.SUPPORTS_ERC20)) { | ||
| coins | ||
| .filter((coin) => coin.family === coinFamily && !coin.isToken) | ||
| .forEach((coin) => { | ||
| const coinNames = { | ||
| Mainnet: `${coin.name}`, | ||
| Testnet: `${coin.name}`, | ||
| }; | ||
|
|
||
| EthLikeErc20Token.createTokenConstructors(coinNames).forEach(({ name, coinConstructor }) => { |
There was a problem hiding this comment.
Can this not be moved inside the previous loop?
There was a problem hiding this comment.
Good idea. Lets put them in one loop.
Fixed the browser test. 22.x is failing for all PRs(most likely due to mismatch versions). |
WIN-6598 TICKET: WIN-6598 chore: add EthLikeErc20 to index.ts for export TICKET: WIN-6598 chore: remove tokenConfig changes and rename feature flag TICKET: WIN-6598 chore: create CoinNames on the go instead of generic TICKET: WIN-6598 chore: fetch coinNames from coin name at runtime TICKET: WIN-6598 feat: one loop for coin and token registration TICKET: WIN-6598 chore: add the browser test for the new token class TICKET: WIN-6598
In progress. Do not review.
WIN-6598
TICKET: WIN-6598